RHAIENG-1649: feat: add automated dependency lock generation script #2644
Conversation
github-actions
bot
added
the
review-requested
WalkthroughAdds a Bash script that discovers Python projects, validates Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
scripts/pylocks_generator.sh (3)
7-9: Quote the$indexvariable for consistency and defensive coding.While the unquoted expansion on line 97 works correctly (the URL has no spaces and expands as a single argument), it's inconsistent with other quoted variables in the command and less defensive. Quote it for consistency.
$HAS_CPU && run_lock "cpu" "$CPU_INDEX" $HAS_CUDA && run_lock "cuda" "$CUDA_INDEX" $HAS_ROCM && run_lock "rocm" "$ROCM_INDEX"Then in the
run_lockfunction:uv pip compile pyproject.toml \ --output-file "$output" \ --format pylock.toml \ --generate-hashes \ --emit-index-url \ --python-version="$PYTHON_VERSION" \ --python-platform linux \ --no-annotate \ - $index + "$index"Also applies to: 97-97
101-107: Consider capturing and logginguv pip compilestderr for better diagnostics.When
uv pip compilefails on line 98, the error output is not captured or logged. This makes it difficult for users to diagnose failures when they occur (e.g., missing dependencies, invalid Python version, malformed pyproject.toml).Consider capturing stderr to help users debug:
if [ $status -ne 0 ]; then warn "${flavor^^} lock failed in $TARGET_DIR" warn "Stderr output: $(uv pip compile ... 2>&1 || true)" rm -f "$output" DIR_SUCCESS=false elseAlternatively, document that users can re-run the command manually with full output for debugging.
24-25: Add validation for targeted directory input.When a user provides a targeted directory via command-line argument (line 25), the script doesn't validate that the directory exists or contains
pyproject.toml. This leads to opaque failures later (e.g., duringcdor whenuv pip compilecan't findpyproject.toml).Add validation after line 25:
if [ $# -gt 0 ]; then TARGET_DIRS=("$1") if [ ! -d "$1" ] || [ ! -f "$1/pyproject.toml" ]; then error "Target directory '$1' does not exist or does not contain pyproject.toml" exit 1 fiThis provides immediate, actionable feedback to users.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/pylocks_generator.sh(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: code-static-analysis
🔇 Additional comments (1)
scripts/pylocks_generator.sh (1)
1-2: Solid error handling setup.The shebang and
set -euo pipefailestablish safe defaults that will catch and exit on errors, undefined variables, and pipeline failures.
atheo89
force-pushed
the
RHAIENG-1489
branch
from
0061bce to
a649bff
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
scripts/pylocks_generator.sh (1)
90-116: Quote the$indexvariable to prevent word-splitting.Line 105 uses an unquoted
$indexvariable in theuv pip compilecommand. Although the current index URLs don't contain spaces, this is fragile and violates defensive shell scripting practices. Unquoted variables can cause word-splitting if they ever contain spaces or special characters.Apply this diff to quote the variable:
uv pip compile pyproject.toml \ --output-file "$output" \ --format pylock.toml \ --generate-hashes \ --emit-index-url \ --python-version="$PYTHON_VERSION" \ --python-platform linux \ --no-annotate \ - $index + "$index"
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/pylocks_generator.sh(1 hunks)
🔇 Additional comments (3)
scripts/pylocks_generator.sh (3)
24-27: ✅ Pre-flight check foruvis now in place.The script now properly detects if the
uvcommand is available and provides a clear error message with installation guidance. This addresses the past critical concern.
118-128: Flavor processing and result aggregation looks good.The conditional execution of
run_lockfor each detected flavor is efficient, and the DIR_SUCCESS flag correctly aggregates per-flavor results (marking the entire directory as failed if any flavor fails). Directory tracking and cleanup are proper.
139-153: Summary reporting is clear and user-friendly.The final summary cleanly separates successful and failed directories, provides actionable remediation guidance for failures (comment out missing packages, report to aipcc team), and is formatted for easy scanning. This aligns well with the PR objectives.
atheo89
force-pushed
the
RHAIENG-1489
branch
from
a649bff to
bad8743
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jiridanek The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
jiridanek
changed the title
…m to use whatever we need according our needs
|
New changes are detected. LGTM label has been removed. |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Makefile(3 hunks)scripts/pylocks_generator.sh(1 hunks)
🔇 Additional comments (2)
Makefile (2)
119-119: Addition ofrefresh-lock-filesto skip-init-for list is correct.This prevents the lock-refresh target from requiring the
bin/buildinputsprerequisite, which is appropriate since refreshing lock files does not need Dockerfile dependency analysis.
412-425: Newrefresh-lock-filestarget is well-structured.The target properly documents usage, accepts optional
DIRandINDEX_MODEparameters, and cleanly delegates to the script. WhenDIRis empty, the script auto-discovers projects fromMAIN_DIRS, which is the intended behavior.Note: Ensure the critical ROOT_DIR fix (from the earlier comment) is applied before this target is used, to guarantee paths are resolved correctly.
atheo89
force-pushed
the
RHAIENG-1489
branch
from
797e6b7 to
8a3910b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
scripts/pylocks_generator.sh (1)
177-185: Minor style note: unquoted$indexvariable.While the index URLs are safely formed without spaces, quoting
"$index"would be a defensive best practice:- $index + "$index"Not critical here, but improves robustness against future changes.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Makefile(3 hunks)scripts/pylocks_generator.sh(1 hunks)
🔇 Additional comments (8)
scripts/pylocks_generator.sh (5)
59-62: Preflight check foruvcommand is in place.The script correctly verifies
uvavailability and exits cleanly with a helpful error message if missing. ✓
128-134: Python version extraction validation is in place.The regex check
^[0-9]+\.[0-9]+$correctly validates the major.minor format and skips invalid directories with a clear warning. ✓
159-200:run_lockfunction correctly handles errors and cleanup.The pattern of
set +e→ capture$?→set -eis the right way to catch exit codes in strict mode. Cleanup on failure (line 191) prevents partial artifacts. ✓
119-219: Main loop logic is sound.Directory discovery, Python version validation, Dockerfile detection, and per-flavor lock generation are all correctly sequenced. The
cd "$TARGET_DIR" || continuepattern safely handles permission or access issues. ✓
236-244: Exit code handling correctly reports failures to CI/Make.The script exits with status 1 if any lock generation fails (line 243), allowing Make targets and CI pipelines to properly detect and fail on errors. ✓
Makefile (3)
28-29: INDEX_MODE variable correctly defined without redefining ROOT_DIR.The prior issue of ROOT_DIR redefinition is fixed. INDEX_MODE is appropriately defined with clear comment. ✓
118-118:refresh-lock-filescorrectly added toskip-init-forlist.This prevents buildinputs from being unnecessarily triggered when running lock generation, which is the correct behavior. ✓
411-424: Newrefresh-lock-filestarget is well-structured.The target:
- Accepts optional
DIRparameter for repo-wide (default) or directory-specific lock generation- Includes comprehensive usage documentation
- Correctly invokes
scripts/pylocks_generator.shwith bothINDEX_MODEandDIR- Uses
@cd $(ROOT_DIR)to ensure correct working directoryIntegration with the Bash script is clean and follows the documented API. ✓
atheo89
added
the
tide/merge-method-squash
atheo89
changed the title
atheo89
force-pushed
the
RHAIENG-1489
branch
from
8a3910b to
7d3732a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Makefile (1)
424-424: Quote variables in shell invocation for robustness.Variables passed to the shell script should be quoted to handle edge cases (e.g., if DIR contains spaces or special characters). While INDEX_MODE is Makefile-controlled and safe, DIR is user-provided.
- @cd $(ROOT_DIR) && bash scripts/pylocks_generator.sh $(INDEX_MODE) $(DIR) + @cd $(ROOT_DIR) && bash scripts/pylocks_generator.sh "$(INDEX_MODE)" "$(DIR)"This follows the pattern used elsewhere in the Makefile (e.g., line 86:
--dockerfile '$(2)') and prevents potential word-splitting or globbing issues.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Makefile(3 hunks)scripts/pylocks_generator.sh(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (31)
- GitHub Check: build (runtime-cuda-pytorch-llmcompressor-ubi9-python-3.12, 3.12, linux/amd64, false) / build
- GitHub Check: build (rocm-jupyter-pytorch-ubi9-python-3.12, 3.12, linux/amd64, false) / build
- GitHub Check: build (cuda-jupyter-tensorflow-ubi9-python-3.12, 3.12, linux/arm64, false) / build
- GitHub Check: build (runtime-cuda-tensorflow-ubi9-python-3.12, 3.12, linux/amd64, false) / build
- GitHub Check: build (runtime-cuda-tensorflow-ubi9-python-3.12, 3.12, linux/arm64, false) / build
- GitHub Check: build (runtime-datascience-ubi9-python-3.12, 3.12, linux/amd64, false) / build
- GitHub Check: build (cuda-jupyter-tensorflow-ubi9-python-3.12, 3.12, linux/amd64, false) / build
- GitHub Check: build (jupyter-minimal-ubi9-python-3.12, 3.12, linux/ppc64le, false) / build
- GitHub Check: build (cuda-rstudio-c9s-python-3.12, 3.12, linux/amd64, false) / build
- GitHub Check: build (jupyter-trustyai-ubi9-python-3.12, 3.12, linux/amd64, false) / build
- GitHub Check: build (codeserver-ubi9-python-3.12, 3.12, linux/arm64, false) / build
- GitHub Check: build (runtime-cuda-pytorch-ubi9-python-3.12, 3.12, linux/amd64, false) / build
- GitHub Check: build (rocm-jupyter-tensorflow-ubi9-python-3.12, 3.12, linux/amd64, false) / build
- GitHub Check: build (jupyter-datascience-ubi9-python-3.12, 3.12, linux/amd64, false) / build
- GitHub Check: build (jupyter-datascience-ubi9-python-3.12, 3.12, linux/ppc64le, false) / build
- GitHub Check: build (jupyter-minimal-ubi9-python-3.12, 3.12, linux/amd64, false) / build
- GitHub Check: build (runtime-datascience-ubi9-python-3.12, 3.12, linux/s390x, false) / build
- GitHub Check: build (runtime-minimal-ubi9-python-3.12, 3.12, linux/s390x, false) / build
- GitHub Check: build (codeserver-ubi9-python-3.12, 3.12, linux/amd64, false) / build
- GitHub Check: build (rocm-runtime-tensorflow-ubi9-python-3.12, 3.12, linux/amd64, false) / build
- GitHub Check: build (cuda-jupyter-pytorch-llmcompressor-ubi9-python-3.12, 3.12, linux/amd64, false) / build
- GitHub Check: build (rocm-runtime-pytorch-ubi9-python-3.12, 3.12, linux/amd64, false) / build
- GitHub Check: build (cuda-jupyter-minimal-ubi9-python-3.12, 3.12, linux/amd64, false) / build
- GitHub Check: build (cuda-jupyter-pytorch-ubi9-python-3.12, 3.12, linux/amd64, false) / build
- GitHub Check: build (jupyter-minimal-ubi9-python-3.12, 3.12, linux/s390x, false) / build
- GitHub Check: build (rocm-jupyter-minimal-ubi9-python-3.12, 3.12, linux/amd64, false) / build
- GitHub Check: build (runtime-minimal-ubi9-python-3.12, 3.12, linux/amd64, false) / build
- GitHub Check: build (cuda-jupyter-minimal-ubi9-python-3.12, 3.12, linux/arm64, false) / build
- GitHub Check: build (rstudio-c9s-python-3.12, 3.12, linux/amd64, false) / build
- GitHub Check: build (cuda-rstudio-rhel9-python-3.12, 3.12, linux/amd64, true) / build
- GitHub Check: build (rstudio-rhel9-python-3.12, 3.12, linux/amd64, true) / build
🔇 Additional comments (1)
scripts/pylocks_generator.sh (1)
1-248: Excellent work addressing all prior concerns—script is production-ready.All three critical issues flagged in previous reviews have been properly resolved:
- ✅ Pre-flight check for
uvcommand availability (lines 63–66)- ✅ Python version extraction validation with regex (lines 133–138)
- ✅ Exit code signaling on lock generation failures (line 247)
The script demonstrates solid bash practices: safe file-finding patterns (
-print0withread -d ''), proper error handling (set +e/set -e), helpful colored output, clear error messages, and comprehensive documentation. The separation of public-index vs. aipcc-index logic and per-flavor lock generation is clean and maintainable.
|
@atheo89: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
3 participants
Related to: https://issues.redhat.com/browse/RHAIENG-1489
and partially to: https://issues.redhat.com/browse/RHAIENG-1649
Description
This PR introduces
pylock_generator.sh, a Bash automation tool that generatesuvlock files (pylock.toml) for all supported notebook image flavors and runtimes.Key features:
aipcc-index→ Uses internal Red Hat AIPCC wheel indexes and generates per-flavor locks (uv.lock/pylock..toml). Andpublic-index→ Uses public PyPI index and updates pylock.toml in-place in the project directory. This Index is currently by default as our odh images are not yet ready to use aipcc wheels. Once ready we can simply tune the to INDEX_MODE=aipcc-index.How can be used
Using the makefile recipe or run the script directly.
Modes:
Run for in automatic mode (repo-wide)
via makefile:
gmake refresh-lock-filesOr the script like:bash scripts/pylocks_generator.shSpecify the index you desire (for now default is the public pypi)
gmake refresh-lock-files INDEX_MODE=aipcc-indexorbash scripts/pylocks_generator.sh aipcc-indexLock using public index and specific directory:
gmake refresh-lock-files INDEX_MODE=aipcc-index DIR=jupyter/minimal/ubi9-python-3.12orbash scripts/pylocks_generator.sh public-index jupyter/minimal/ubi9-python-3.12--
NOTE: If there is a missing package on
pyproject.tomlit will produces the following(for example):so in this case you can comment out problematic package in the above case the kubeflow-training, and run it again, and probably it will print another missing package, and so on. So these missing packages should be reported to AIPCC team.
Merge criteria:
Summary by CodeRabbit